Repository navigation
fix(openapi): document optional sign request UUID headers - #9466
vitormattos wants to merge 3 commits into
Conversation
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Code Metrics ReportNote: Metrics describe the revision analyzed by the successful PHPUnit run. Newer commits with pending, canceled or failed checks may not yet be represented in this report.
Details | | main (642daf6) | #9466 (a8fa04b) | +/- |
|---------------------|----------------|-----------------|------|
| Coverage | 76.4% | 76.4% | 0.0% |
| Files | 512 | 512 | 0 |
| Lines | 28845 | 28845 | 0 |
| Covered | 22063 | 22063 | 0 |
+ | Test Execution Time | 3m42s | 3m13s | -29s |Code coverage of files in pull request scope (55.4% → 55.4%)
Reported by octocov |
|
/backport to stable35 |
backportbot-libresign
Bot
added
the
backport-request
|
/backport to stable34 |
|
/backport to stable33 |
lfals
left a comment
lfals
left a comment
There was a problem hiding this comment.
The six public operations that already accept libresign-sign-request-uuid and skip it for a logged-in caller now declare that header as optional. Generated OpenAPI and TypeScript types match those attributes.
One regression: OpenApiRequestHeadersTest requires sign_file-request-code-by-file-id to stay undocumented, so a client that follows the spec is rejected with Invalid UUID before requestCodeByFileId runs.
| /** The optional header applies only to methods that can identify a request without a route UUID. */ | ||
| private const OPERATIONS = [ | ||
| 'signature_elements-create-signature-element', | ||
| 'signature_elements-get-signature-elements', | ||
| 'signature_elements-use-saved-signature-elements', | ||
| 'signature_elements-preview-signature-element', | ||
| 'signature_elements-patch-signature-element', | ||
| 'signature_elements-delete-signature-element', | ||
| 'id_docs-add-files', | ||
| 'id_docs-delete', | ||
| 'id_docs-list-unauthenticated-signer-documents', | ||
| ]; |
There was a problem hiding this comment.
sign_file-request-code-by-file-id consumes libresign-sign-request-uuid, and this allow-list requires that header to stay undocumented.
SignFileController::requestCodeByFileIdis#[RequireSigner]onPOST /api/{apiVersion}/sign/file_id/{fileId}/code, which has no{uuid}(SignFileController.php:491).InjectionMiddleware::requireSigner()readsgetParam('uuid', header), andSignerValidator::validateUuidFormat('')throws Invalid UUID before the action runs. A logged-in caller is not exempt (InjectionMiddleware.php:317).ModalVerificationCode.requestCodeposts this route whenfileIdis set and sends neither the header noruuid.
Please confirm this operation should stay off the contract, or add the same optional header and include it in OPERATIONS:
'sign_file-request-code-by-file-id',| $present[] = $id; | ||
| self::assertSame(self::SIGN_REQUEST_HEADER, $headers[0]['name']); | ||
| self::assertSame(self::DESCRIPTION, $headers[0]['description']); | ||
| self::assertFalse($headers[0]['required'] ?? false, "The header is optional for {$id}"); |
There was a problem hiding this comment.
signature_elements-use-saved-signature-elements is not optional for the caller who can succeed.
useSavedSignatureElementsis#[RequireSignRequestUuid]with noskipIfAuthenticated, andPOST /signature/elements/savedhas no{uuid}and no documented query or body UUID (SignatureElementsController.php:185).- An anonymous email signer with no header and no
uuidparameter gets Invalid UUID fromvalidateUuidFormatbefore the action runs. - A logged-in caller still fails in the action with Saved signatures are only available to signers identified by email, even when the UUID is valid.
- The six headers added here use
skipIfAuthenticated: true, so optional matches those.
Please confirm this is the intended product behaviour, or stop asserting required === false for signature_elements-use-saved-signature-elements and mark that header required.
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Summary
#[RequestHeader(indirect: true)]to the six public OCS operations that already support the optionallibresign-sign-request-uuidviaInjectionMiddlewarebut omitted it from OpenAPI:SignatureElementsController::{previewSignatureElement,patchSignatureElement,deleteSignatureElement}andIdDocsController::{addFiles,delete,listUnauthenticatedSignerDocuments}.OpenAPI::SCOPE_IGNOREcontrollers.openapi.json,openapi-full.json, and corresponding TypeScript types. Administration scope is unchanged. No multipart changes (fix(api): detect multipart file uploads and reject truncated batches (#9435) #9452 / Document multipart uploads in the OpenAPI contract #9435).Evidence
in: headerstring parameter.composer openapi: passed using lockednextcloud/openapi-extractorv1.9.1 and an isolated PHP/Node toolchain.npm run typescript:generate: passed.vendor/bin/phpunit -c tests/php/openapi/phpunit.xml: 23 tests, 406 assertions, passed.npm run ts:check: passed.npm run lint: passed with 23 pre-existing warnings and no errors.Limitations / follow-up review
lib/base.php) is not present. Execute in a disposable Nextcloud 36 runtime; include negative authorization and valid guest/authenticated paths before merging.psalm.xmlreferences missing Nextcloud core paths. CI must verify Psalm; no suppression was introduced.x-prefix for this custom header; renaming it would break existing clients, so the existing wire name is retained.Two incremental commits are DCO-signed and cryptographically verified. Do not merge until CI and runtime authorization checks are reviewed.